test: compute the per-file coverage table from the tracefile (#974) - #975
Conversation
…prompt#974) The nightly's 'least covered first' table printed columnar_parquet_codec.c | 2.0% 100|3200% 2| - 0 for a file whose records say LF:100 LH:100 FNF:2 FNH:2 BRF:64 BRH:47. A file at 100% presented as 2.0% and ranked the worst in the tree, a function rate above 100% on its face, and an empty branch column where the tracefile carries 13,900 branches. Four of the files it named as least covered were between 94% and 100%. It came from lcov --list. The same step's lcov --summary, four lines earlier on the same tracefile, was correct -- and so is lcov --list here on the run's own uploaded tracefile. The builds differ only in distro patch level (the runner installs 2.0-4ubuntu2; this was checked on 2.0-1) and which patch does it has not been bisected. So the table is computed. A rate is a division of two integers the tracefile states outright, and LF: cannot be got wrong by a patch to --list. Cross-checked against lcov --list on all 39 files in that tracefile and independently against the raw counters: no disagreement in either. lcov --summary keeps its job. The format carries hit/found rather than only the total, because '93.7% 22765' cannot be checked by a reader and '93.7% 21320/22765' can. An absent counter prints '-', not 0.0%: a header with no branches at 0.0% sorts to the top and reads as the least covered file in the tree. TWELVE ARMS DRIVE THE GENERATOR over a synthetic tracefile rather than grepping it, because a static check that the runner calls the right script cannot tell whether the script is correct, and 'the table is wrong' was the defect. Four of those arms failed when first written, and each was the arm's fault: - two matched MY OWN COMMENTS in the runner explaining the replaced call -- a guard defeated by the sentence documenting it, and the third instance today of a comment carrying a token tripping a grep that wanted the call. They count over code with comments stripped now, under a premise that stripping comments did not strip the code. - one asserted a cell by field number; absent cells collapse to one token each, so the dashes are $4 and $5, not $6. The property is asserted first and the position second. - one matched a substring: 0\.0% is inside 50.0%, which is the line rate of the fixture that has no branches. Anchored, with a control over a genuinely zero-covered file proving the anchor did not defeat the assertion. harness_selftest 815 checks 0 FAIL, docs_style 9/9, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
…mandprompt#974) Twelve new check names in harness_selftest, which the ledger covers, so the gate refuses the PR until the ledger has seen them. commandprompt#972 merged first, so this is the PR that paid the rebase. The sequence was: drop my earlier ledger commit, rebase onto main, re-run the suite on the REBASED tree, guard that log, merge it, and DERIVE the census from the resulting file. The derived value happens to equal main's 847 plus twelve, and it is not set that way -- the number has only ever been right on purpose when read back from the ledger. rows 847 -> 859 checks_never_observed_red 847 -> 859 awk -F'\t' '$4=="never"' | wc -l suites_not_covered 250 unchanged; this adds no suite Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
bfd4b05 to
c302c67
Compare
|
Rebased onto The sequence, which is exactly what I predicted on #432 two hours ago and is worth having on the record as the measured cost rather than the estimate: The derived value is 859, which happens to equal main's 847 plus my twelve — and it is not set that way. Had I written 859 by addition it would have been right today and wrong the first time two ledger-touching PRs land in an order I did not predict. That has already happened once today: I derived against 846, #972 landed first, and the number I had was stale before it was reviewed. 816 rather than 815, because the rebase brought in #972's new premise check as well as my twelve. The CHANGELOG conflict#972's entry is on main where mine inserts. Both kept, in landing order, with each asserted to appear exactly once in the result — the identity check rather than an eyeball, because a fold loses one silently and the file is long enough that nobody would notice. Unchanged by the rebase
Two PRs, thirteen checks, fourteen steps of taxThat is the number #432 was missing. Not an argument against seeding — an argument that the ledger's key shape has to be settled before 240 suites pay this on every PR, because changing it afterwards means regenerating 240 suites' rows while everyone in the meantime pays the tax against a key already known to be wrong. |
|
I ran That gap is worth naming plainly: #975 changes the script the nightly's coverage job executes, and the thirteen checks on this PR do not execute it. The twelve arms in and the table, on a tracefile this run produced rather than one I downloaded: The coverage is low because only Two things that only an end-to-end run could show:
And The prefix, because an instrumented build is a write to shared stateThe run installs a Gate, unchanged13/13 at |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approved at c302c670. Driven on a tracefile carrying the exact record you cite, not read.
The defect and the fix, reproduced
Fed the generator LF:100 LH:100 FNF:2 FNH:2 BRF:64 BRH:47 — the record the nightly rendered as 2.0% 100 | 3200% 2:
file lines functions branches
zero.c 0.0% 0/50 0.0% 0/4 -
half.c 50.0% 100/200 50.0% 5/10 -
columnar_parquet_codec.c 100.0% 100/100 100.0% 2/2 73.4% 47/64
nobranch.c 100.0% 10/10 100.0% 1/1 -
A file at 100% reads as 100%, and it sorts to the bottom of a list headed "least covered first" rather than the top. The hit/found format is the part I'd defend hardest: 93.7% 22765 cannot be checked by a reader and 93.7% 21320/22765 can, and this whole issue exists because nobody checked.
Edges I attacked
LF:0 LH:0 - no division by zero
no counters at all -
LF:notanumber - the int() guard holds
empty tracefile rc=1, "no SF records ... nothing to rank"
missing file rc=1
a file with no branches - NOT 0.0%, so it does not sort to the top
That last one is the quiet correctness in the change: - and 0.0% are different facts, and conflating them would put every branch-free file at the head of the worst-covered list.
One finding, non-blocking, and it is the PR's own argument turned on itself
An impossible rate is rendered as excellent coverage and sorted out of sight.
SF:/src/d_impossible.c
LF:2
LH:7
a_worst.c 1.0% 1/100
b_mid.c 50.0% 50/100
c_good.c 99.0% 99/100
d_impossible.c 350.0% 7/2 <- last, i.e. "best covered in the tree"
e_nodata.c -
350.0% 7/2 is not a coverage figure; hit > found is arithmetically impossible. The table prints it without comment and ranks it below a genuinely 99%-covered file.
The nightly's 3200% is exactly this signal, and it went unread for however long the table has been wrong. At the top of the list it was missed; at the bottom it would be missed more surely. Your docstring makes the case better than I can — "a reader who can check the number is the only one who will notice when it is wrong again" — and a reader cannot check 7/2 against anything, because it is not wrong by a little.
Cheap fix if you want it: treat hit > found as a distinct state, print it as impossible or suffix it, and sort it with the worst rather than the best. I would not hold the PR for it; a corrupt tracefile is not reachable from lcov's own writer, which is why this is a finding and not a blocker.
The --list removal, counted over code
Your own trap, checked the way you taught it two hours ago:
--list, whole file 1 <- the comment explaining the removal
--list, code only 0
--summary, code 1 correct on both builds, so it stays
pgc_coverage_table.py 1 wired in
Your four faults are the reason I trust the twelve arms
Two arms matched your own comment rather than the call, one asserted the wrong field because absent cells collapse to one token, and the fourth was the substring trap — 0\.0% matching inside 50.0%, which was the line rate of your own fixture. You then ran a control with a genuinely zero-covered file to prove the anchor had not defeated the assertion.
An anchor is an instrument and needs its own negative case. That control is the part most people skip, and it is the difference between a tightened pattern and a broken one.
I could not reproduce your 39-file cross-check here — lcov --summary produces nothing against a synthetic tracefile whose SF: paths do not exist, and the container carries 2.0-1, the build that was already correct. Yours was done against the nightly's own uploaded tracefile, which is the right artifact, and the arithmetic is independently checkable from the records regardless.
…ommandprompt#961) Eleven new check names in harness_selftest, which the ledger covers. THIRD TIME TODAY, and the second time this PR has paid it: commandprompt#975 merged while this was open, so the census I had derived against 847 was stale before anyone reviewed it. Dropped that commit, rebased onto f69ed1e, re-ran the suite on the REBASED tree, guarded that log, merged it, and DERIVED the census from the file. rows 859 -> 870 checks_never_observed_red 859 -> 870 awk -F'\t' '$4=="never"' | wc -l The derived value equals main's 859 plus eleven and is not set that way. Writing it by addition would have been right today and wrong the moment two ledger PRs land in an order nobody predicted -- which is exactly what happened to the first version of this commit. commandprompt#978 is still open and also moves this line. If it lands first, this rebases again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
#975 landed first, so this PR pays the tax second. Done in the order @OffgridwithJD established, because the earlier census was derived against 847 and was stale before anyone reviewed it: rebase onto f69ed1e (6 commits replayed, no conflict) FRESH log on the REBASED tree, not the pre-rebase one GUARD that log before it goes near the ledger merge it DERIVE the census from the resulting file The guard, because a log is an input and an input is a claim: verdict=PASSED fails=0 checks=846 unrunnable=0 records=846 records == checks run, and the total is above a floor DERIVED, NOT ADDED. The merge reports rows=889 and the census is counted from the file: 889 rows, 889 never, 0 ever red, and the partition closes. It happens to equal 859 + 30. Writing that by addition would have been right today and wrong the first time a merge order surprises me, which has now happened to the other PR in this queue twice in one afternoon. Thirty rows added, zero removed, all in 450-a-red-nightly-must-be-findable. Ceiling unchanged at 250: these checks are in a suite already covered, so nothing about coverage moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbyGSaU93XYQr8aH4NrUiw
…ommandprompt#961) THIRD pass for this PR. commandprompt#975 moved the census while it was open, then commandprompt#978 moved it again, so each earlier derivation was stale before anyone could review it. That is not a defect in the PRs -- it is what a committed census costs when more than one change is in flight, and it is the measurement commandprompt#432 needed. Done in the agreed order: drop the stale ledger commit, rebase onto main, FRESH log from the rebased tree, guard it, merge, then DERIVE from the file. The guard now also asserts RESULT records == checks run, which catches a truncated log -- @linuxhikerpm's addition, and the one my earlier guards missed. No expected census value was written down before the count ran. Knowing the target does not bias the count; it biases whether a DISAGREEMENT is investigated or dismissed as a mis-run, and that is where an expected answer wins. The selftest part is byte-identical to the approved revision: a rebase that altered it would be an approval riding through a change to the mechanism it was granted for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
2 participants
Closes #974. The nightly's per-file coverage table named the best-covered files as the worst, and the table is now computed from the tracefile instead of parsed out of
lcov --list.A file at 100% presented as 2.0% and sorted to the top of a list headed least covered first. A function rate above 100% on its face. An empty branch column where the tracefile carries 13,900 branches. Four of the files it named as least covered were between 94% and 100%.
Why, as far as I established it
The table came from
lcov --list. The same step'slcov --summary, four lines earlier on the same tracefile, was correct — and so islcov --listrun here on the run's own uploadedcoverage.info. The two builds differ only in distro patch level: the runner installs2.0-4ubuntu2, this was checked on2.0-1.I did not bisect which patch does it and I am not guessing. The point of the change is that the output was wrong and the input was fine, so the fix is to stop depending on the part that varies.
What replaces it
test/pgc_coverage_table.pyreadsLF/LH,FNF/FNH,BRF/BRHand divides. Those counters are stated outright in the tracefile and cannot be got wrong by a patch to--list.Verified two ways, not one:
Checking against another tool alone would only prove the two agree; checking against the raw counters proves the arithmetic.
lcov --summaryis untouched. It was right on both builds and it is what the summary block uses.Two deliberate choices about the format
hit/found, not just the total.
93.7% 22765cannot be checked by a reader;93.7% 21320/22765can. A reader who can check the number is the only one who will notice when it is wrong again — which is the whole reason this sat in the log for two nights.An absent counter prints
-, never0.0%. A header with no branches shown at 0.0% sorts to the top and reads as the least covered file in the tree. That is how a table misleads while every individual number in it is defensible.Twelve arms, and four of them were wrong first
They drive the generator over a synthetic tracefile rather than grepping for the call, because a static check that the runner invokes the right script cannot tell whether the script is correct, and "the table is wrong" was the defect. The fixture carries one partially covered file, one at 100%, and one with no branch counter at all.
Four failed when first written, each the arm's fault and not the generator's:
no lcov --list survives→ got 1lcov --summary stays→ wanted 1, got 2an absent counter prints a dash$6; absent cells collapse to one token each, so they are$4and$5never reads as 0.0%→ got 10\.0%is a substring of50.0%, the line rate of the no-branch fixtureThe first two are the third instance today of a comment carrying a token tripping a grep that wanted the call — a
pgc_summarymention in #969 and ashellcheckmention in #972 were the others. They now count over code with comments stripped, under a premise that stripping comments did not strip the code, since a grep over an emptied file reports the same0as a grep that matches nothing.The fourth is anchored to
(^|[^0-9.])0\.0%, with a control: a genuinely zero-covered fixture still matches, so the anchor narrowed the pattern without defeating the assertion.Gate
The registered list came from
bash test/run_all_versions.sh --list-suites(252, clean), not from a grep of theSUITESarray — four of my extractions of that array returned 560, 333, 284 and 267 names, because its closing paren is not the first^)after it.One thing whoever merges needs to know
#972 moves
checks_never_observed_redon the same line. Whichever of the two merges second needs a rebase and the census re-derived on the other's value, not added to it. #972 takes it to 847; this one takes 846 to 858; the correct result after both is whateverawk -F'\t' '$4=="never"' | wc -lsays on the merged ledger, which is the only way that number has ever been right on purpose.Closes #974.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a